Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate reshare permissions against actual path that the user tries to share #27820

Closed
wants to merge 4 commits into from
Closed

Validate reshare permissions against actual path that the user tries to share #27820

wants to merge 4 commits into from

Conversation

juliusknorr
Copy link
Member

Otherwise this could lead to taking the wrong user mount in case there
are multiple ones with different permissions that the user could use to
reshare

Possible reproduction:

  • A creates a folder and shares it with group without reshare permissions
  • A creates a subfolder inside of the share and shares it to B with reshare permisions
  • B is member of group
  • B Tries to create a share link of the subfolder

Before:
💥 Cannot increase permissions

After:
The proper source user mount is taken for permission comparison where the user has reshare permissions and the share link creation passes.

@juliusknorr juliusknorr added 2. developing Work in progress bug labels Jul 6, 2021
@juliusknorr
Copy link
Member Author

juliusknorr commented Jul 6, 2021 •
edited
Loading

Other bug found, to be filed separately:

  • B is offered resharing in the UI for the subfolder (but not children) when accessing it through the original share path, but the backend doesn't allow resharing there

@juliusknorr
Copy link
Member Author

Unit tests should be fine now and were actually using the wrong source node in case of a reshare as then the share would always be created from the resharing users filesystem instead of using the owners user folder as a base.

@juliusknorr juliusknorr added 3. to review Waiting for reviews feature: sharing and removed 2. developing Work in progress labels Sep 14, 2021
@juliusknorr juliusknorr marked this pull request as ready for review September 14, 2021 17:10
@juliusknorr juliusknorr added this to the Nextcloud 23 milestone Sep 14, 2021
@nickvergessen
Copy link
Member

We really need integrations tests for this.

@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 15, 2021
@juliusknorr juliusknorr force-pushed the bugfix/noid/check-reshare-permission-mountpoint branch from 627640e to 09a6305 Compare October 20, 2021 14:13
@juliusknorr juliusknorr added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 20, 2021
@juliusknorr
Copy link
Member Author

Will look into some tests for that and will double check the suggestion from @PVince81

@juliusknorr
Validate reshare permissions against actual path that the user tries …
b5bf60f 
…to share

Otherwise this could lead to taking the wrong user mount in case there
are multiple ones with different permissions that the user could use to
reshare

Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Fix tests to use the proper node of the user creating the share
70845cf 
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Add test case
62f507e 
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Copy link
Member Author

Pushed another attempt as the share node might be the one located in the owners filesystem

server/lib/private/Share20/Share.php

Lines 182 to 186 in 215aef3

if ($this->userManager->userExists($this->shareOwner)) {
$userFolder = $this->rootFolder->getUserFolder($this->shareOwner);
} else {
$userFolder = $this->rootFolder->getUserFolder($this->sharedBy);
}
compared to the previously assumed shared by users filesystem https://github.com/nextcloud/server/blob/master/lib/private/Share20/Manager.php#L271-L275

@juliusknorr juliusknorr force-pushed the bugfix/noid/check-reshare-permission-mountpoint branch from 8d8664e to 20fc6a7 Compare October 28, 2021 09:55
@juliusknorr juliusknorr force-pushed the bugfix/noid/check-reshare-permission-mountpoint branch from 20fc6a7 to a6c557c Compare October 28, 2021 10:50
@blizzz blizzz mentioned this pull request Apr 13, 2022
Merged
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
Merged
Merged
This was referenced Aug 24, 2022
Merged
Merged
This was referenced Sep 6, 2022
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
This was referenced Sep 20, 2022
Closed
Merged
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@young-coder-vn

This comment was marked as abuse.

Sign in to view
@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Merged
@blizzz blizzz mentioned this pull request Mar 7, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
Merged
Closed
Merged
@blizzz blizzz mentioned this pull request May 17, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
@juliusknorr
Copy link
Member Author

Closing as I don't have time to properly finish this

@juliusknorr juliusknorr closed this Nov 6, 2023
@nickvergessen nickvergessen deleted the bugfix/noid/check-reshare-permission-mountpoint branch November 6, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 left review comments

@skjnldsv skjnldsv skjnldsv approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees
No one assigned
Labels
2. developing Work in progress bug feature: sharing
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.
6 participants
@juliusknorr @nickvergessen @young-coder-vn @PVince81 @skjnldsv @blizzz